Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-raise encountered exceptions in pilot tests #2754

Merged
merged 13 commits into from
Jun 12, 2023
Merged

Conversation

darrenburns
Copy link
Member

@darrenburns darrenburns commented Jun 8, 2023

When an exception was raised in a pilot test, it prevented logic in _wait_for_screen from working correctly (counters would never decrement to zero). The result was that when exceptions occurred, it always timed out.

We now record the first exception that occurs in an app, and we re-raise that exception in the __aexit__ of the context manager returned by run_test, so that tests fail and test frameworks will print the stack trace.

We now detect if an exception occurs and set an event. If the event is set, then _wait_for_screen won't wait for the timeout to be reached allowing it to exit early.

  • Docstrings on all new or modified functions / classes
  • Updated documentation
  • Updated CHANGELOG.md (where appropriate)

@darrenburns darrenburns marked this pull request as ready for review June 8, 2023 15:24
@davep
Copy link
Contributor

davep commented Jun 8, 2023

To keep in mind for when this PR settles and/or is ready to merge: the fix that's in here for OptionList needs to keep #2567 in mind. The fix applied here pretty much reverts the change made for #2567 -- which is fair as that change was questionable at best but solved the immediate problem.

So when this lands we need to be 100% sure that it doesn't reintroduce #2557 and if it does (I suspect it might actually), thanks to #2582 being fixed, we can likely actually fix #2557 using the rebuild in _on_show approach.

@darrenburns
Copy link
Member Author

I've confirmed that this doesn't break Select and doesn't reintroduce #2557.

Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, just one question.

src/textual/app.py Show resolved Hide resolved
@darrenburns darrenburns merged commit b699733 into main Jun 12, 2023
@darrenburns darrenburns deleted the pilot-exceptions branch June 12, 2023 15:42
rodrigogiraoserrao added a commit that referenced this pull request Mar 25, 2024
This is probably an edge-case that wasn't covered in the original PR that introduced the machinery (namely App._exception and App._exception_event) that I want to leverage here.

Related PRs: #2754
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants